Skip to content

fix(subflows): recurse into all descendants for lock, enable, and protection checks#3412

Merged
waleedlatif1 merged 8 commits intostagingfrom
fix/z-index
Mar 4, 2026
Merged

fix(subflows): recurse into all descendants for lock, enable, and protection checks#3412
waleedlatif1 merged 8 commits intostagingfrom
fix/z-index

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Lock/unlock and enable/disable now propagate to all nested descendants, not just direct children
  • Protection checks walk the full ancestor chain instead of only checking the immediate parent
  • Extracted shared isBlockProtected and findAllDescendantNodes utilities to eliminate 7+ duplicate inline closures
  • Fixed the same direct-children-only bugs in the socket persistence layer (caused lock state to be lost on refresh)
  • Fixed z-index issues for nested subflow interaction

Type of Change

  • Bug fix

Testing

Tested manually with 3-level nesting (Parallel > Parallel > Parallel > Function). All existing tests pass (158/158).

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 4, 2026 11:25pm

Request Review

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@cursor
Copy link

cursor bot commented Mar 4, 2026

Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR fixes a class of bugs where lock/enable-disable operations only propagated to direct children instead of all nested descendants, and where protection checks only inspected the immediate parent rather than the full ancestor chain.

Core improvements:

  • isBlockProtected / isAncestorProtected now walk the full parentId chain with a visited Set for cycle safety, replacing the old single-parent check.
  • findAllDescendantNodes refactored from recursive to iterative BFS with cycle detection.
  • Shared utilities eliminate 7+ duplicate inline closures across store, collaborative-workflow hook, socket operations, and editor panel.
  • Lock/enable/delete operations now correctly protect and traverse all descendants through the utility functions.
  • Edge-label z-index raised to 1001 to stay above child nodes at z-index 1000.
  • elevateNodesOnSelect={false} combined with manual zIndex: 1000 for child blocks to preserve visual hierarchy.

Files with significant logic changes:

  • store.ts, utils.ts: Core protection and descendant utilities
  • socket/database/operations.ts: Mirrors store logic for DB-side operations
  • use-collaborative-workflow.ts: Uses shared utilities for undo/redo state capture
  • editor.tsx, block-protection-utils.ts: Switched to shared isBlockProtected / isAncestorProtected
  • subflow-node.tsx: Updated selection ring styling and click-catching overlay
  • workflow.tsx: Refactored ReactFlow conditional mounting and z-index management

The utility extraction and ancestor-chain fixes are well-implemented with proper cycle detection. Edge-label z-index was appropriately adjusted to accommodate the child node elevation.

Confidence Score: 4/5

  • Core logic is sound with proper cycle detection and comprehensive descendant traversal; all existing tests pass (158/158).
  • The utility extraction and ancestor-chain fixes are well-implemented. The refactoring from recursive to iterative BFS with visited Set prevents infinite loops. Protection checks now correctly walk the full ancestor chain across store, socket, and collaborative layers. Edge-label z-index was appropriately bumped to 1001 to stay above child nodes at z-index 1000. Manual test coverage with 3-level nesting confirms the nested subflow behavior works as intended.
  • No files require special attention based on filtered review findings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Lock / Enable / Delete operation\n(ids[])"] --> B{For each id}
    B --> C["isBlockProtected(id, blocks)"]
    C -->|"walks full ancestor chain\n(visited Set for cycle safety)"| D{Protected?}
    D -->|Yes| E["Skip block entirely"]
    D -->|No| F["Add block to operation set"]
    F --> G{Is loop or parallel?}
    G -->|Yes| H["findAllDescendantNodes(id, blocks)\n(iterative BFS, visited Set)"]
    H --> I{For each descId}
    I --> J["isBlockProtected(descId, blocks)"]
    J -->|Protected| K["Skip descendant"]
    J -->|Not protected| L["Add descendant to operation set"]
    G -->|No| M["Apply operation to set"]
    L --> M
    M --> N["Persist via store / socket / DB"]

    subgraph "Utility Sources"
        U1["stores/workflows/workflow/utils.ts\n  isBlockProtected\n  isAncestorProtected\n  findAllDescendantNodes"]
        U2["socket/database/operations.ts\n  isDbBlockProtected\n  findDbDescendants\n  (mirror for raw DB records)"]
    end
Loading

Last reviewed commit: bf6104f

…p code

- Add canvasReadyRef to skip container dimension recalculation during
  ReactFlow init — position changes from extent clamping fired before
  block heights are measured, causing containers to resize on page load
- Resolve globals.css merge conflict, remove global z-index overrides
  (handled via ReactFlow zIndex prop instead)
- Clean up subflow-node: hoist static helpers to module scope, remove
  unused ref, fix nested ternary readability, rename outlineColor→ringColor

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…le-toggle

The enable-toggle for descendants was checking only direct `locked` status
instead of walking the full ancestor chain via `isBlockProtected`. This meant
a block nested 2+ levels inside a locked subflow could still be toggled.
Also added TSDoc clarifying why boxShadow works for subflow ring indicators.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cursor
Copy link

cursor bot commented Mar 4, 2026

PR Summary

Medium Risk
Touches core workflow editing/persistence paths (lock/enable/edge/parent updates) and changes how nested containers propagate state, which could affect permissions and destructive operations if the new ancestry/descendant traversal is wrong. UI interaction changes (z-index/pointer-events/selection ring) also risk subtle regressions in nested ReactFlow behavior.

Overview
Nested subflow operations now apply recursively. Lock/unlock, enable/disable, handle toggles, rename protection, edge add/remove validation, deletion, and parent-move checks now treat blocks as protected if any ancestor container is locked, and propagate container operations to all descendants (not just direct children) via shared isBlockProtected/isAncestorProtected and iterative findAllDescendantNodes.

Persistence layer matches the new rules. Socket DB operations add DB-friendly protection/descendant helpers so server-side batch delete/toggle/move/edge ops filter protected targets correctly and include all descendants, preventing state loss on refresh for deeply nested subflows.

ReactFlow nesting interaction is stabilized. Subflow nodes get a click-catching body for selection, child blocks inside containers are forced above it with zIndex, selection ring styling is switched back to boxShadow for consistency, edge label z-index is raised, and canvas config tweaks disable node elevation on select and simplify the loading overlay/background styling.

Written by Cursor Bugbot for commit bf6104f. Configure here.

waleedlatif1 and others added 2 commits March 4, 2026 12:14
The canvasReadyRef gating in onNodesChange didn't fully fix the
container resize-on-load issue. Reverting to address properly later.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Leftover from merge conflict resolution — not part of this PR's changes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…cked, restore fade-in transition

isAncestorLocked was derived from isBlockProtected which short-circuits
on block.locked, so a self-locked block inside a locked ancestor showed
"Unlock block" instead of "Ancestor container is locked". Now walks the
ancestor chain independently.

Also restores the accidentally removed transition-opacity duration-150
class on the ReactFlow container.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…e-toggle, restore edge-label z-index

The top-level block check in batchToggleEnabled used block.locked (self
only) while descendants used isBlockProtected (full ancestor chain). A
block inside a locked ancestor but not itself locked would bypass the
check. Now all three layers (store, collaborative hook, DB operations)
consistently use isBlockProtected/isDbBlockProtected at both levels.

Also restores the accidentally removed edge-labels z-index rule, bumped
from 60 to 1001 so labels render above child nodes (zIndex: 1000).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Additional Comments (1)

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx
Internal ReactFlow element background colours no longer explicitly set

Three selectors were removed from reactFlowStyles:

  • [&_.react-flow__pane]:!bg-[var(--bg)]
  • [&_.react-flow__renderer]:!bg-[var(--bg)]
  • [&_.react-flow__viewport]:!bg-[var(--bg)]

The bg-[var(--bg)] class was added to the ReactFlow component's own wrapper, but the inner .react-flow__pane, .react-flow__renderer, and .react-flow__viewport divs are now unset. In ReactFlow v11 these elements can have their own default background that bleeds through, especially in dark mode. If this was tested and the internal defaults happen to be transparent (and the wrapper colour shows through correctly), it would be worth a brief comment confirming that assumption so future contributors don't re-add the selectors unnecessarily.

…on to all traversals

- Extract isAncestorProtected from utils.ts so editor.tsx doesn't
  duplicate the ancestor-chain walk. isBlockProtected now delegates to it.
- Add visited-set cycle detection to all ancestor walks
  (isBlockProtected, isAncestorProtected, isDbBlockProtected) and
  descendant searches (findAllDescendantNodes, findDbDescendants) to
  guard against corrupt parentId references.
- Document why click-catching div has no event bubbling concern
  (ReactFlow renders children as viewport siblings, not DOM children).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@waleedlatif1 waleedlatif1 merged commit 6b355e9 into staging Mar 4, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/z-index branch March 4, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant